Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adds SAGP as an experimental expo plugin feature #4440

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Jan 13, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Adds SAGP as an experimental expo plugin when the enableAndroidGradlePlugin is enabled in the experimental_android section of the @sentry/react-native/expo plugin.
The supported options are the following:

  • autoUploadProguardMapping
  • includeProguardMapping
  • dexguardEnabled
  • uploadNativeSymbols
  • autoUploadNativeSymbols
  • includeNativeSources
  • includeSourceContext

💡 Motivation and Context

Fixes #4400

💚 How did you test it?

CI, Manual testing:

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

See getsentry/sentry-android-gradle-plugin#809

Copy link
Contributor

github-actions bot commented Jan 13, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 75bd2f0

Copy link
Contributor

github-actions bot commented Jan 13, 2025

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1219.34 ms 1228.73 ms 9.39 ms
Size 3.19 MiB 4.26 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
43e66e0+dirty 1247.39 ms 1253.46 ms 6.07 ms
d2c32bb+dirty 1244.00 ms 1245.77 ms 1.77 ms
fe13591+dirty 1250.69 ms 1246.27 ms -4.43 ms
22e31b6+dirty 1276.55 ms 1278.12 ms 1.57 ms
8b86336+dirty 1236.26 ms 1235.00 ms -1.26 ms
c2a4e9b+dirty 1247.39 ms 1243.04 ms -4.35 ms
eb1e19f+dirty 1229.91 ms 1231.63 ms 1.71 ms
80b2ce3+dirty 1245.12 ms 1262.04 ms 16.92 ms
1faf8e3+dirty 1223.38 ms 1220.56 ms -2.82 ms
7f6950b+dirty 1250.94 ms 1252.92 ms 1.98 ms

App size

Revision Plain With Sentry Diff
43e66e0+dirty 2.92 MiB 3.67 MiB 772.38 KiB
d2c32bb+dirty 2.92 MiB 3.64 MiB 742.84 KiB
fe13591+dirty 2.92 MiB 3.66 MiB 757.71 KiB
22e31b6+dirty 2.92 MiB 3.43 MiB 524.74 KiB
8b86336+dirty 3.19 MiB 4.25 MiB 1.06 MiB
c2a4e9b+dirty 2.92 MiB 3.64 MiB 739.91 KiB
eb1e19f+dirty 2.92 MiB 3.64 MiB 742.82 KiB
80b2ce3+dirty 2.92 MiB 3.40 MiB 492.75 KiB
1faf8e3+dirty 2.92 MiB 3.64 MiB 742.61 KiB
7f6950b+dirty 2.92 MiB 3.67 MiB 772.53 KiB

Previous results on branch: antonis/experimental-expo-sagp

Startup times

Revision Plain With Sentry Diff
c5d03fc+dirty 1230.90 ms 1235.12 ms 4.22 ms
e165be8+dirty 1228.63 ms 1245.41 ms 16.78 ms
28fa62b+dirty 1214.88 ms 1214.82 ms -0.06 ms
a05231a+dirty 1225.98 ms 1234.84 ms 8.86 ms
fc3cd14+dirty 1252.76 ms 1248.33 ms -4.43 ms
e0596ce+dirty 1235.27 ms 1230.63 ms -4.63 ms

App size

Revision Plain With Sentry Diff
c5d03fc+dirty 3.19 MiB 4.25 MiB 1.06 MiB
e165be8+dirty 3.19 MiB 4.25 MiB 1.06 MiB
28fa62b+dirty 3.19 MiB 4.25 MiB 1.06 MiB
a05231a+dirty 3.19 MiB 4.25 MiB 1.06 MiB
fc3cd14+dirty 3.19 MiB 4.25 MiB 1.07 MiB
e0596ce+dirty 3.19 MiB 4.25 MiB 1.07 MiB

Copy link
Contributor

github-actions bot commented Jan 13, 2025

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1231.96 ms 1229.65 ms -2.31 ms
Size 2.63 MiB 3.70 MiB 1.06 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
43e66e0+dirty 1226.82 ms 1225.92 ms -0.90 ms
d2c32bb+dirty 1223.69 ms 1229.49 ms 5.80 ms
fe13591+dirty 1208.25 ms 1219.53 ms 11.28 ms
22e31b6+dirty 1253.62 ms 1265.96 ms 12.34 ms
8b86336+dirty 1219.38 ms 1230.73 ms 11.36 ms
c2a4e9b+dirty 1240.10 ms 1239.22 ms -0.88 ms
eb1e19f+dirty 1209.56 ms 1214.94 ms 5.38 ms
80b2ce3+dirty 1265.92 ms 1268.60 ms 2.69 ms
1faf8e3+dirty 1214.87 ms 1222.83 ms 7.97 ms
7f6950b+dirty 1235.53 ms 1227.30 ms -8.23 ms

App size

Revision Plain With Sentry Diff
43e66e0+dirty 2.36 MiB 3.11 MiB 759.78 KiB
d2c32bb+dirty 2.36 MiB 3.08 MiB 737.22 KiB
fe13591+dirty 2.36 MiB 3.10 MiB 752.40 KiB
22e31b6+dirty 2.36 MiB 2.87 MiB 520.67 KiB
8b86336+dirty 2.63 MiB 3.68 MiB 1.05 MiB
c2a4e9b+dirty 2.36 MiB 3.08 MiB 734.00 KiB
eb1e19f+dirty 2.36 MiB 3.08 MiB 737.21 KiB
80b2ce3+dirty 2.36 MiB 2.84 MiB 486.98 KiB
1faf8e3+dirty 2.36 MiB 3.08 MiB 736.75 KiB
7f6950b+dirty 2.36 MiB 3.11 MiB 760.04 KiB

Previous results on branch: antonis/experimental-expo-sagp

Startup times

Revision Plain With Sentry Diff
c5d03fc+dirty 1214.23 ms 1214.45 ms 0.21 ms
e165be8+dirty 1226.43 ms 1225.46 ms -0.97 ms
28fa62b+dirty 1222.57 ms 1223.91 ms 1.34 ms
a05231a+dirty 1232.37 ms 1242.12 ms 9.75 ms
fc3cd14+dirty 1220.68 ms 1209.53 ms -11.15 ms
e0596ce+dirty 1220.34 ms 1213.90 ms -6.44 ms

App size

Revision Plain With Sentry Diff
c5d03fc+dirty 2.63 MiB 3.69 MiB 1.05 MiB
e165be8+dirty 2.63 MiB 3.69 MiB 1.05 MiB
28fa62b+dirty 2.63 MiB 3.69 MiB 1.05 MiB
a05231a+dirty 2.63 MiB 3.68 MiB 1.05 MiB
fc3cd14+dirty 2.63 MiB 3.69 MiB 1.05 MiB
e0596ce+dirty 2.63 MiB 3.69 MiB 1.06 MiB

Copy link
Contributor

github-actions bot commented Jan 13, 2025

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 361.73 ms 392.37 ms 30.64 ms
Size 7.15 MiB 8.38 MiB 1.23 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d16beca+dirty 389.49 ms 423.86 ms 34.37 ms
34aba08+dirty 331.79 ms 376.69 ms 44.91 ms
acadc0f+dirty 259.04 ms 304.67 ms 45.63 ms
52a8031+dirty 330.72 ms 358.76 ms 28.03 ms
3261206+dirty 426.43 ms 446.20 ms 19.78 ms
e5c9b8b+dirty 335.40 ms 360.06 ms 24.67 ms
4161236+dirty 429.89 ms 498.74 ms 68.85 ms
c830127+dirty 352.35 ms 388.96 ms 36.61 ms
abb7058+dirty 320.78 ms 324.08 ms 3.30 ms
5571a20+dirty 359.52 ms 389.80 ms 30.28 ms

App size

Revision Plain With Sentry Diff
d16beca+dirty 7.15 MiB 8.37 MiB 1.22 MiB
34aba08+dirty 7.15 MiB 8.07 MiB 946.13 KiB
acadc0f+dirty 7.15 MiB 8.03 MiB 903.20 KiB
52a8031+dirty 7.15 MiB 8.09 MiB 965.95 KiB
3261206+dirty 7.15 MiB 8.38 MiB 1.23 MiB
e5c9b8b+dirty 7.15 MiB 8.10 MiB 980.41 KiB
4161236+dirty 7.15 MiB 8.38 MiB 1.23 MiB
c830127+dirty 7.15 MiB 8.38 MiB 1.23 MiB
abb7058+dirty 7.15 MiB 8.10 MiB 980.40 KiB
5571a20+dirty 7.15 MiB 8.20 MiB 1.05 MiB

Previous results on branch: antonis/experimental-expo-sagp

Startup times

Revision Plain With Sentry Diff
e165be8+dirty 403.17 ms 433.17 ms 30.00 ms
a05231a+dirty 410.49 ms 464.94 ms 54.45 ms
28fa62b+dirty 467.78 ms 451.33 ms -16.45 ms
c5d03fc+dirty 400.53 ms 450.84 ms 50.31 ms
e0596ce+dirty 393.94 ms 461.88 ms 67.94 ms
fc3cd14+dirty 409.87 ms 463.46 ms 53.59 ms

App size

Revision Plain With Sentry Diff
e165be8+dirty 7.15 MiB 8.38 MiB 1.23 MiB
a05231a+dirty 7.15 MiB 8.38 MiB 1.23 MiB
28fa62b+dirty 7.15 MiB 8.38 MiB 1.23 MiB
c5d03fc+dirty 7.15 MiB 8.38 MiB 1.23 MiB
e0596ce+dirty 7.15 MiB 8.38 MiB 1.23 MiB
fc3cd14+dirty 7.15 MiB 8.38 MiB 1.23 MiB

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 426.49 ms 434.58 ms 8.09 ms
Size 17.75 MiB 20.11 MiB 2.36 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
40c35c5 438.38 ms 437.52 ms -0.86 ms
1c9e040 325.02 ms 311.57 ms -13.45 ms
9672577 416.34 ms 421.26 ms 4.92 ms
cdf2bdf 448.42 ms 443.42 ms -5.00 ms
79976dd 456.94 ms 439.17 ms -17.76 ms
d361d38 354.10 ms 381.69 ms 27.59 ms
18ce5e8 450.83 ms 436.08 ms -14.75 ms
b95b8af 454.05 ms 454.53 ms 0.48 ms
d197b5c+dirty 338.94 ms 354.87 ms 15.93 ms
1332acb 493.40 ms 475.20 ms -18.21 ms

App size

Revision Plain With Sentry Diff
40c35c5 17.75 MiB 20.11 MiB 2.37 MiB
1c9e040 17.75 MiB 20.11 MiB 2.37 MiB
9672577 17.75 MiB 20.11 MiB 2.36 MiB
cdf2bdf 17.74 MiB 20.10 MiB 2.36 MiB
79976dd 17.75 MiB 20.11 MiB 2.36 MiB
d361d38 17.73 MiB 19.81 MiB 2.08 MiB
18ce5e8 17.74 MiB 20.10 MiB 2.36 MiB
b95b8af 17.73 MiB 20.11 MiB 2.37 MiB
d197b5c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
1332acb 17.74 MiB 20.09 MiB 2.35 MiB

Previous results on branch: antonis/experimental-expo-sagp

Startup times

Revision Plain With Sentry Diff
a05231a 459.71 ms 465.70 ms 5.99 ms
c5d03fc 422.04 ms 418.83 ms -3.21 ms
fc3cd14 453.14 ms 472.16 ms 19.02 ms
28fa62b 420.92 ms 423.72 ms 2.80 ms
e165be8 451.06 ms 466.90 ms 15.84 ms
e0596ce 405.46 ms 423.43 ms 17.97 ms

App size

Revision Plain With Sentry Diff
a05231a 17.75 MiB 20.11 MiB 2.36 MiB
c5d03fc 17.75 MiB 20.11 MiB 2.37 MiB
fc3cd14 17.75 MiB 20.11 MiB 2.37 MiB
28fa62b 17.75 MiB 20.11 MiB 2.37 MiB
e165be8 17.75 MiB 20.11 MiB 2.37 MiB
e0596ce 17.75 MiB 20.11 MiB 2.37 MiB

@antonis antonis marked this pull request as ready for review January 14, 2025 07:36
import { withAppBuildGradle, withProjectBuildGradle } from '@expo/config-plugins';

export interface SentryAndroidGradlePluginOptions {
sentryAndroidGradlePluginVersion?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's bake in a version that's compatible with current RN SDK and keep it our responsibility instead of users having to pick SAGP version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with 2539a24 to hardcode the SAGP version and use enableAndroidGradlePlugin as a flag to enable.

Comment on lines 19 to 28
export function withSentryAndroidGradlePlugin(config: any, options: SentryAndroidGradlePluginOptions = {}): any {
const version = options.sentryAndroidGradlePluginVersion;
const includeProguardMapping = options.includeProguardMapping ?? true;
const dexguardEnabled = options.dexguardEnabled ?? false;
const autoUploadProguardMapping = options.autoUploadProguardMapping ?? true;
const uploadNativeSymbols = options.uploadNativeSymbols ?? true;
const autoUploadNativeSymbols = options.autoUploadNativeSymbols ?? true;
const includeNativeSources = options.includeNativeSources ?? true;
const includeSourceContext = options.includeSourceContext ?? false;
const autoInstallationEnabled = options.autoInstallationEnabled ?? false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can set the defaults when deconstructing the options argument, it makes it easier to read and removes the names repetition.

Suggested change
export function withSentryAndroidGradlePlugin(config: any, options: SentryAndroidGradlePluginOptions = {}): any {
const version = options.sentryAndroidGradlePluginVersion;
const includeProguardMapping = options.includeProguardMapping ?? true;
const dexguardEnabled = options.dexguardEnabled ?? false;
const autoUploadProguardMapping = options.autoUploadProguardMapping ?? true;
const uploadNativeSymbols = options.uploadNativeSymbols ?? true;
const autoUploadNativeSymbols = options.autoUploadNativeSymbols ?? true;
const includeNativeSources = options.includeNativeSources ?? true;
const includeSourceContext = options.includeSourceContext ?? false;
const autoInstallationEnabled = options.autoInstallationEnabled ?? false;
export function withSentryAndroidGradlePlugin(
config: any,
{
sentryAndroidGradlePluginVersion,
includeProguardMapping = true,
dexguardEnabled = false,
autoUploadProguardMapping = true,
uploadNativeSymbols = true,
autoUploadNativeSymbols = true,
includeNativeSources = true,
includeSourceContext = false,
autoInstallationEnabled = false
}: SentryAndroidGradlePluginOptions = {}
): any {
const version = sentryAndroidGradlePluginVersion;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 🙇
Updated with 59e54ce

const withSentryAppBuildGradle = (config: any): any => {
return withAppBuildGradle(config, (config: any) => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
if (config.modResults.language === 'groovy') {
Copy link
Member

@krystofwoldrich krystofwoldrich Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's invert this and use it as a guard !== groovy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍 Updated with 7b053f5

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
config.modResults.contents = contents;
} else {
throw new Error('Cannot configure Sentry in android/app/build.gradle because it is not in Groovy.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the warnOnce helper instead of throw. It will make the message more readable for the users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍
Updated with b1230bd


// Modify android/build.gradle
const withSentryProjectBuildGradle = (config: any): any => {
return withProjectBuildGradle(config, (projectBuildGradle: any) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also check if the lang is groovy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point 👍 Added a check with 9d6d9d2

Comment on lines 43 to 46
projectBuildGradle.modResults.contents = projectBuildGradle.modResults.contents.replace(
/dependencies\s*{/,
`dependencies {\n ${dependency}`,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add a warning if we could not add the dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added with cf55ce4 and f04a2c7

@krystofwoldrich
Copy link
Member

When this is merged and release we should also update the docs.

Maybe a sub page to https://docs.sentry.io/platforms/react-native/manual-setup/expo/

(not to interrupt the default setup flow with the gradle options).

@antonis
Copy link
Collaborator Author

antonis commented Jan 16, 2025

When this is merged and release we should also update the docs.

Maybe a sub page to https://docs.sentry.io/platforms/react-native/manual-setup/expo/

(not to interrupt the default setup flow with the gradle options).

Makes sense 👍 I followed up with a docs PR for this.

@lucas-zimerman
Copy link
Collaborator

When this is merged and release we should also update the docs.

Maybe a sub page to https://docs.sentry.io/platforms/react-native/manual-setup/expo/

(not to interrupt the default setup flow with the gradle options).

It would be great if we could merge this Page and make it only available to people accessing it only by direct link, so it is easier to validate the URLs and also keep it hidden from the users while an official release isn't released

Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no issues with the PR, LGTM!

@antonis
Copy link
Collaborator Author

antonis commented Jan 30, 2025

When this is merged and release we should also update the docs.

Maybe a sub page to https://docs.sentry.io/platforms/react-native/manual-setup/expo/

(not to interrupt the default setup flow with the gradle options).

It would be great if we could merge this Page and make it only available to people accessing it only by direct link, so it is easier to validate the URLs and also keep it hidden from the users while an official release isn't released

The current doc PR adds it as a subpage with no link from the parent (other than the navigation links). I'm ok with hiding it. Wdyt @krystofwoldrich?

ps. If I'm not mistaken hiding the page can be achieved by setting draft and noindex to true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proguard Mappings are not uploaded using expo plugin
3 participants